Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to avoid static symbols if leaving them would make a leak #15548

Merged
merged 1 commit into from
Jul 20, 2022

Conversation

KacperFKorban
Copy link
Member

Possible fix for #9314

Typer.ensureNoLocalRefs doesn't avoid local static refs like anonymous
class instances to allow typing more specific expected types with e.g. refinements.
Unfortunately this can cause leaks when expected types have to be
inferred. This PR tries to fix this problem by adding a fallback that
avoids all local symbols if this leak were to happen.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd try to simply drop the isStatic condition in avoid and see whether that works.
If it doesn;t, we'd need more explanations in the comments why the particular code path that sets avoidStatic to true is different form all the others.

compiler/src/dotty/tools/dotc/core/TypeOps.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/typer/Typer.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/typer/Typer.scala Outdated Show resolved Hide resolved
@odersky odersky assigned KacperFKorban and unassigned odersky Jul 5, 2022
@@ -512,7 +512,7 @@ object TypeOps:
@threadUnsafe lazy val forbidden = symsToAvoid.toSet
def toAvoid(tp: NamedType) =
val sym = tp.symbol
!sym.isStatic && forbidden.contains(sym)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But shouldn't the anonymous subclass be considered non-static since it's declared in a block? I assume the issue is that its owner is set to the top-level scope, but if we can't change its owner we could perhaps special case ANON_CLASS in SymDenotation#isStatic

@KacperFKorban
Copy link
Member Author

@odersky It seems that removing the isStatic check doesn't break any of the tests (and fixes this one).

@smarter I checked and adding a special case for ANON_CLASS also doesn't break anything. But shouldn't it actually be static though, since it's owner is static? If so then someone can potentially rely on this check in the future.

@odersky odersky merged commit a163c3f into scala:main Jul 20, 2022
@Kordyjan Kordyjan added this to the 3.2.1 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants